-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Primitive enum conversion reform #3040
base: master
Are you sure you want to change the base?
Conversation
Currently this tends to be done using `as`, which has two flaws: 1. It will silently truncate. e.g. if you have a `repr(u64)` enum, and you write `value as u8` (perhaps because the repr changed), you get no warning or error that truncation will occur. 2. You cannot use enums in code which is generic over `From` or `Into`. Instead, by allowing a `From` (and accordingly `Into`) implementation to be easily derived, we can avoid these issues. There are a number of crates which provide macros to provide this implementation, as well as `TryFrom` implementations. I believe this is enough of a foot-gun that it should be rolled into `std`. See rust-lang/rfcs#3040 for more information.
f931ac4
to
731db83
Compare
This RFC proposes making it easier to use `From`, `Into`, and potentially `TryFrom` for conversions between enums with primitive representations, and their associated primitives. It optionally also proposes removing or restricting `as` conversion between these types. [Rendered](https://github.com/illicitonion/rfcs/blob/enum-as-to-into/text/3040-enum-as-to-into.md)
731db83
to
f1a5d48
Compare
My core question for this one is why it should happen for potentially-truncating conversions from enums but not from integer types. It's not obvious to me that enum conversions specifically are particularly error prone than the other ones. Another potential alternative: provide direct access to the correctly-typed discriminant value with a new derivable trait, and encourage use of that instead of |
I agree that many of these ideas could, and probably should, apply equally to integer types. I think the core difference is that with enums There are three "severities" of solution I think we can aim at:
Personally, I would aim for at least 2, ideally also 3, and that these should apply to enum and integer types equally.
Thanks for the suggestion - I've added it as an alternative. I think I lean more towards the consistency of |
Moving away from the My only concern is the Is there precedent for naming a derive something other than the relevant type? |
As far as I can tell, not in std/core. Of the crates linked from #2783 (comment) the most common solution is to use a An alternative could be something like |
I was thinking |
I don't think a lint like Logically speaking, why shouldn't I think a new trait for converting a value to the underlying/wrapped type/representation of its type would be more useful than this RFC because it would cover not only enums but also struct newtypes (that may wrap a single integer or whatever type). This new trait wouldn't suffer from the type inference issue that Update: Update 2: let value: u16 = e.into() + 1;
// --^^^^--
// | |
// | cannot infer type for type parameter `T` declared on the trait `Into`
// this method call resolves to `T` ...but with the unstable type_ascription feature, you can write the following: let value = e.into(): u16 + 1; |
There is some value in removing
Making enum-to-integer conversion opt-in via (I'm not any team member; I got here from This Week in Rust 370.) |
Having conversion traits would be great. The changes to |
Thanks for your input, all! I put together #3046 to propose a new |
I think the current RFC text uses the term "truncate" incorrectly perhaps throughout but at least in a couple of places under the "Likely incorrect casts" heading. I believe that whenever an integer is cast to a narrower integer type by using the @illicitonion Due to the fact that I don't see how a truncating cast where the resulting mathematical value is the same as the original could cause any issues, I think that we should keep the status quo otherwise except that we should add two new lints:
For example: #[repr(u64)]
enum Number {
Zero = 0,
Thousand = 1000,
}
const NUMBER_ZERO: Number = Number::Zero;
const ZERO: u64 = 0;
const THOUSAND: u64 = 1000;
// `non_preserving_integer_cast` would apply to these two lines and warn by default:
let _ = Number::Thousand as u8;
let _ = THOUSAND as u8;
// But `non_preserving_integer_cast` would NOT apply to these three lines:
let _ = Number::Zero as u8;
let _ = NUMBER_ZERO as u8;
let _ = ZERO as u8;
let number = if it_is_tuesday {
Number::Zero
} else {
Number::Thousand
};
// `potentially_non_preserving_enum_cast` would apply to this line and warn by default:
let _ = number as u8;
// But `potentially_non_preserving_enum_cast` would NOT apply to this line:
let _ = number as u16;
// Also, `potentially_non_preserving_enum_cast` would NOT apply to these three lines:
let _ = Number::Zero as u8;
let _ = NUMBER_ZERO as u8;
let _ = ZERO as u8; Update: #[repr(u32)]
enum Number {
Zero = 0,
Million = 1_000_000,
}
fn foo(items: &[i8], number: Number) {
// `potentially_non_preserving_enum_cast` would apply to the following line if and only
// if `usize` cannot represent `1_000_000` (on platforms with 16-bit pointers), whereas
// `From`/`Into` is no help because `usize` doesn't implement `From<u32>`, so this
// wouldn't work: `items[usize::from(number.into_underlying())]`.
let _ = items[number as usize];
} |
I'm generally familiar with the terms "widening" and "narrowing" for the size-changing aspects of casts, and "truncating" for the discarding data aspect. This is in line with Mitre's definition of truncation: https://cwe.mitre.org/data/definitions/197.html Is there a term you think is more generally known/applicable for describing this kind of cast?
No, I'm specifically referring to cases where the value is changed. I agree that where only narrowing is performed, and values don't change, the operation is likely to be correct.
I think this would be a great lint to add, where it can be identified. I suspect that in practice it would trigger very rarely compared to the "potential" version, but I would definitely support it!
Am I reading this correctly that this is identical to the
If all of the measures in this RFC are implemented, including removing let _ = items[number.into_underlying() as usize]; or let _ = items[u32::from(number) as usize]; But if we stopped short of that, I believe this case would be covered by the proposed |
I stand corrected. You were using the term correctly. Thank you for the definition - I was unable to find a definition for "truncate" in programming context, so I then read too much into a line in The Rustonomicon that says "casting from a larger integer to a smaller integer (e.g. u32 -> u8) will truncate".
No, it's not identical because my suggested lint takes advantage of the fact that the compiler knows the value of each enum variant at compile time and is also allowed to assume that an enum variable may only have a bit pattern that is identical to the bit pattern of one of the enum type's variants. In the RFC text you give the following example: #[repr(u16)]
enum Number {
Zero,
One,
}
fn main() {
let bad = Number::Zero as u8;
// ^^^ This cast may truncate the value of `bad`, as it is represented by a `u16` which may not fit in a `u8`.
} ...whereas my suggested A better word for the lint would be
No, it wouldn't be covered by it, or at least I'm not proposing a lint that would warn about casting an integer variable to a narrower integer type because I think those warnings would be way too ubiquitous. My other proposed And if there's a warn-by-default lint like Update: |
This RFC proposes making it easier to use
From
,Into
, andpotentially
TryFrom
for conversions between enums with primitiverepresentations, and their associated primitives. It optionally also
proposes removing or restricting
as
conversion between these types.Rendered